I think this has a symlink race unless we manage to pass O_NOFOLLOW to fs.open. That wouldn't let realroot have symlinks though (unless there's a way to call openat?) and I don't know how to pass our intention to send.

Hi Greg,

I'm not sure what you mean. I see that it is theoretically possible to serve a file that we should disallow, if a request has determined that there is no symlink, and in the interim the filesystem is changed to include a symlink... but this would have to happen in the very small period of time between when node returns from line 109 and when it executes line 126 and would only result in a file served if the path existed as a valid path (without symlinks) in the first place, in which case, we would have been serving it prior anyway.

Is that what you are referring to? If you can explain your concern I'll attempt to mitigate it.

Jay

Right -- classic TOCTOU. Further protecting us is that the check you're doing is synchronous, so you'd have to be running something like pm2 or kubernetes to be vulnerable.

That said, followsymlinks is intended to protect against an attacker that has the ability to create symlinks, (otherwise there wouldn't be a vulnerable symlink in the first place) and our check (this PR) is running in response to a request that the attacker can generate, (the HTTP request for the static content) so the attacker is control of the timing of both the creation of the symlink and the validation of it.

It looks like that var stream = send(req, path, opts) is ultimately doing fs.createReadStream(path, options), (https://github.com/pillarjs/send/blob/master/index.js#L796) so if we set opts.flags = O_RDONLY & O_NOFOLLOW, we'd get the protection we seek. I don't see a openat (http://man7.org/linux/man-pages/man2/open.2.html) equivalent for createReadStream so we wouldn't be able to have a symlink in the realroot.

Let me know if that works, or if you'd like me to test something out.

Thanks for working on this PR!

I've updated the patch to pass flags O_NOFOLLOW and O_RDONLY when followsymlinks is false. I also changed it so that if you choose 'followsymlinks=false' then it will resolve the root path to the real path at startup, which should still allow symlinks in the root path, at the cost of locking the root path at startup time. With followsymlinks=true (the default) the behavior is unchanged from prior to the patch.

Is anyone able to merge this? @dougwilson?